Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add support for 'json_name' annotation #408

Merged
merged 2 commits into from
Nov 24, 2021
Merged

feat: Add support for 'json_name' annotation #408

merged 2 commits into from
Nov 24, 2021

Conversation

ajaykarthikr
Copy link
Collaborator

Inspired by #210, this PR adds support for json_name as per @stephenh's feedback on that PR.

Hm, my interpretation of json_name attribute would be that this particular name would still be name and then other_name would only show up on the JSON on the wire (i.e. in toJson methods) and then looked for when data is parsed in from the wire (i.e. in the fromJson methods).

I made changes to generateFromJson and generateToJson functions and created a new function determineFieldJsonName for json field names. If the json_name is not present in the proto file it uses the name attribute for json field names.

There's also a simple unit test for testing this feature.

stephenh
stephenh approved these changes Nov 23, 2021
View reviewed changes
src/utils.ts Outdated Show resolved Hide resolved
@stephenh
Copy link
Owner

@ajaykarthikr thanks, looks great! One small question about the snakeToCamel check, but otherwise we can merge and release. Thanks!

@ajaykarthikr
Copy link
Collaborator Author

ajaykarthikr commented Nov 24, 2021 •
edited
Loading

@stephenh I don't think there's a way to know if the user had actually set the json_name field because that field is never empty according to the docs.

JSON name of this field. The value is set by protocol compiler. If the user has set a "json_name" option on this field, that option's value will be used. Otherwise, it's deduced from the field's name by converting it to camelCase.

So, I just removed the snakeToCamel flag check according to your previous feedback. This will lowerCameCase json field names if json_name option is not present.

Anyway, if you read the language guide for proto3, lowerCamelCase is the default behavior. So we should lowerCamelCase it by default for compatibility reasons too.

If there's an usecase, we can probably give an option to the user to dynamically change casing to lowerCamelCase or snake_case. Similar to this betterproto. I can make a seperate PR for this. What do you say?

@stephenh
Copy link
Owner

@ajaykarthikr ah, gotcha, I didn't realize that protoc always fills in the json name for us...

Okay, yeah, I think this PR makes sense / is great.

It is technically a breaking change for users that were using snakeToCamel=false, but I think that's fine for this PR.

If users report breaking changes and want to keep the old behavior, I think maybe turning snakeToCamel into multi-values like true (default) | false (current behavior in this PR) | keys-and-json (new behavior that would keep snake case in JSON).

Going to hit merge; thanks!

@stephenh stephenh merged commit b519717 into stephenh:main Nov 24, 2021
stephenh pushed a commit that referenced this pull request Nov 24, 2021
@semantic-release-bot
chore(release): 1.90.0 [skip ci]
92f6995 
# [1.90.0](v1.89.0...v1.90.0) (2021-11-24)

### Features

*  Add support for 'json_name' annotation ([#408](#408)) ([b519717](b519717))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.90.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@stephenh stephenh mentioned this pull request Nov 27, 2021
Closed
@SchroederChris SchroederChris mentioned this pull request Dec 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenh stephenh stephenh approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ajaykarthikr @stephenh